Skip to content

Make constants in batch submission logic configurable.#396

Merged
philippecamacho merged 4 commits intocelo-integration-rebase-16.2from
philippe/configurable-batch-submission-constants
Apr 7, 2026
Merged

Make constants in batch submission logic configurable.#396
philippecamacho merged 4 commits intocelo-integration-rebase-16.2from
philippe/configurable-batch-submission-constants

Conversation

@philippecamacho
Copy link
Copy Markdown
Collaborator

@philippecamacho philippecamacho commented Apr 6, 2026

Closes https://app.asana.com/1/1208976916964769/project/1209976130071762/task/1213432243074583

This PR:

  • Makes VERIFY_RECEIPT_MAX_BLOCKS, VERIFY_RECEIPT_SAFETY_TIMEOUT, and VERIFY_RECEIPT_RETRY_DELAY runtime-configurable instead of compile-time constants
  • Exposes them as CLI flags (espresso.verify-receipt-max-blocks, espresso.verify-receipt-safety-timeout, espresso.verify-receipt-retry-delay) with corresponding OP_BATCHER_ESPRESSO_VERIFY_RECEIPT_* env vars
  • Preserves the existing values (5 blocks, 5 min, 100ms) as defaults, so deployments that don't set these flags are unaffected

This PR does not:

  • Change any default behavior or tuning values
  • Make other hardcoded values configurable (worker counts, attestation HTTP timeout, queue capacities) — those can be addressed separately if needed
  • Touch the derivation pipeline or any consensus-critical path

How to test this PR

  • CI is green

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces configurable parameters for Espresso batch submission receipt verification, including maximum block wait times, safety timeouts, and retry delays. These settings are integrated into the CLI, the batcher service configuration, and the transaction submission worker logic. Feedback was provided regarding the duplication of default values for these new parameters across different files, suggesting the use of shared constants to improve maintainability.

Comment thread espresso/cli.go Outdated
@philippecamacho philippecamacho marked this pull request as ready for review April 6, 2026 19:20
@claude
Copy link
Copy Markdown

claude Bot commented Apr 6, 2026

Claude finished @philippecamacho's task in 1m 36s —— View job


Code Review: Make constants in batch submission logic configurable

  • Read existing review comments
  • Review changed files
  • Provide review feedback

Overall this is a clean, well-scoped PR. The functional options pattern is followed consistently, defaults are preserved, and the plumbing from CLI → service → driver → submitter is correct. A few observations:

Validation of zero/negative values

The new config fields (VerifyReceiptMaxBlocks, VerifyReceiptSafetyTimeout, VerifyReceiptRetryDelay) aren't validated in CLIConfig.Check(). Setting VerifyReceiptMaxBlocks to 0 would make the block-count check trigger re-submission on every first verification attempt, and a zero VerifyReceiptRetryDelay would cause a tight spin loop in the verify worker. I left an inline comment with a concrete suggestion for adding validation.

Duplicated defaults (echoing Gemini's feedback)

I agree with the existing review thread — the magic values 5, 5 * time.Minute, and 100 * time.Millisecond appear both in cli.go flag definitions and as VERIFY_RECEIPT_* constants in espresso.go. Exporting the constants and referencing them in the CLI flags would eliminate drift risk. Not blocking, but worth addressing.

evaluateVerification method receiver change — good call

Converting evaluateVerification from a free function to a method on *espressoTransactionSubmitter is the right move. It keeps the configurable values close to where they're used without parameter bloat.

No test coverage for the new configurability

There are no tests for evaluateVerification or the new With* options. Since evaluateVerification is a pure-logic decision function (no I/O), it would be straightforward to add table-driven tests exercising the different timeout paths with custom config values. Not a blocker for this PR, but would add confidence.

Minor: constant naming convention

The VERIFY_RECEIPT_* constants use SCREAMING_SNAKE_CASE, which is unusual for Go (idiomatic Go uses MixedCaps). Since these are now only used as internal defaults inside NewEspressoTransactionSubmitter, this is cosmetic and not worth a change in this PR.


Looks good overall — the main actionable item is the input validation.

Comment thread espresso/cli.go
Copy link
Copy Markdown
Member

@jjeangal jjeangal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all good to me!

@philippecamacho philippecamacho force-pushed the philippe/configurable-batch-submission-constants branch from 9adb3bf to 9ae1806 Compare April 7, 2026 17:03
@philippecamacho philippecamacho merged commit deb7985 into celo-integration-rebase-16.2 Apr 7, 2026
63 of 64 checks passed
@philippecamacho philippecamacho deleted the philippe/configurable-batch-submission-constants branch April 7, 2026 18:06
QuentinI pushed a commit that referenced this pull request Apr 9, 2026
* Make constants in batch submission logic configurable.

* Define verify receipt default values as shared exported constants

* Validate verify-receipt tuning flags are non-zero when Espresso is enabled

* Fix integration test failures introduced by verify-receipt validation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants